[WIP] Use mapping format for reading `date` values #169

Yury-Fridlyand · 2022-11-16T21:40:02Z

Signed-off-by: Yury-Fridlyand yuryf@bitquilltech.com

Description

The fix is to collect formats from mapping and pass it to OpenSearchExprValueFactory.
I added mapping2 member to IndexMapping and OpenSearchExprValueFactory which hold required data. The original mapping is not removed yet to ensure that everything works as was before.

Test data

date_formats.json.txt
date_formats_mappings.json.txt

curl -s -XDELETE "http://localhost:9200/date-formats?pretty"
curl -s -H 'Content-Type: application/json' -XPUT "http://localhost:9200/date-formats?pretty" --data-binary @date_formats_mappings.json
curl -s -H 'Content-Type: application/json' -XPOST "http://localhost:9200/date-formats/_bulk?pretty" --data-binary @date_formats.json

Test queries

select * from date-formats;
select <format> from date-formats where name = '<format>';

search source=date-formats;
search source=date-formats | where name='<format>' | fields <format>;

For example, epoch_second (was reported on forum)

opensearchsql> select epoch_second from date-formats where name = 'epoch_second';
fetched rows / total rows = 1/1
+-------------------------------+
| epoch_second                  |
|-------------------------------|
| 1984-04-12 09:07:42.000123456 |
+-------------------------------+

Unsupported formats

weekyear
strict_weekyear
weekyear_year
strict_weekyear
year
strict_year
year_month
strict_year_month

(see links below)
They fall back to error:

opensearch-project-sql/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

Lines 258 to 261 in f335101

    
           // TODO throw exception 
        
           LogManager.getLogger(OpenSearchExprValueFactory.class).error( 
        
               String.format("Can't recognize parsed value: %s, %s", parsed, parsed.getClass())); 
        
           return new ExprStringValue(value.stringValue());

Time interpreting issue

opensearch-project-sql/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

Line 254 in f335101

    
           return new ExprTimestampValue(new ExprTimeValue(LocalTime.from(parsed)).timestampValue());

goes to

opensearch-project-sql/core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java

Line 62 in f335101

return LocalDate.now();

There is no option to access queryContext there.
This could be fixed by one of

Returning ExprTimeValue
Applying time value on top of Epoch or any other fixed date. Note - this should be consistent across the plugin.

TBD

What to do with LocalTime?
What to do with unsupported formats?
Can we return not ExprTimestampValue for date and time columns? Note: ensure that entire column has the same or similar format(s).

TODO

See TODO comments in code
Fix SQLCorrectnessIT with query SELECT Cancelled, AvgTicketPrice, FlightDelayMin, Carrier, timestamp FROM opensearch_dashboards_sample_data_flights. Test results - Class org.opensearch.sql.sql.SQLCorrectnessIT.pdf
Wrong data returned for timestamp, mapping "format" : "yyyy-MM-dd HH:mm:ss"

Links

Issues Resolved

opensearch-project#794
https://forum.opensearch.org/t/sql-select-fails-on-date-fields-format-epoch-second/11521

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

… update tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

MaxKsyunz · 2022-11-17T19:43:30Z

core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java

@@ -57,6 +57,21 @@ public LocalTime timeValue() {
    return time;
  }

+  @Override
+  public LocalDate dateValue() {
+    return LocalDate.now();


What was this required for?

It would need to account for query start time.

We return TIMESTAMP for OpenSearch date type => we return ExprTimestampValue

An index can store time in date field => parser returns LocalTime

To satisfy both 1 and 2 we have to build ExprTimestampValue from LocalTime which calls to LocalDate.now().
I see few ways to fix/avoid this:

Don't return ExprTimestampValue where it is possible. It is nice to have, but still we would have to build ExprTimestampValue from LocalTime.

Change how we build ExprTimestampValue from LocalTime - use a fixed date (e.g. Epoch) instead of today.

Deliver queryContext to OpenSearchExprValueFactory in opensearch module.

Yury-Fridlyand · 2022-11-17T20:01:23Z

@penghuo, @dai-chen, please have a look on this PoC. Your feedback and opinion is highly appreciated.

dai-chen · 2022-11-23T23:20:04Z

...earch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

+  private final Map<ExprType, BiFunction<Content, MappingEntry, ExprValue>> typeActionMap =
+      new ImmutableMap.Builder<ExprType, BiFunction<Content, MappingEntry, ExprValue>>()


Because MappingEntry is only used by date time, I'm think can we add this mapping info to Content?

dai-chen · 2022-11-23T23:31:51Z

opensearch/src/main/java/org/opensearch/sql/opensearch/mapping/MappingEntry.java

+@AllArgsConstructor
+public class MappingEntry {


We may need more thoughts about this new abstraction. For now it seems only for OpenSearch date field mapping. Can we extend it to carry mapping info for all OpenSearch fields? For example, multi-field may use this abstraction to get inner field name and type.

{ "mappings": { "properties": { "city": { "type": "text", "fields": { "raw": { "type": "keyword" } } } } } }

Yes, current implementation doesn't work with keyword and nested types.

dai-chen · 2022-11-23T23:37:03Z

...n/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilder.java

+    namedAggregatorList.forEach(agg -> builder.put(agg.getName(), new MappingEntry(null, null, agg.type())));
+    groupByList.forEach(group -> builder.put(group.getNameOrAlias(), new MappingEntry(null, null, group.type())));


I'm thinking should we refactor OpenSearchDataType? Rather than static Enum, we instantiate it with mapping info for each field during analyzing.

class OpenSearchDataType implements ExprType: val mappingInfo; val osTypeName; ...

@dai-chen,
Please see changes in #180. They don't contain date format processing, but only refactor of OpenSearchDataType.

Yury-Fridlyand · 2023-06-29T23:00:46Z

Superseded by #273

Yury-Fridlyand added 3 commits November 16, 2022 13:30

First PoC.

f335101

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Replace old typeMapping by new one in OpenSearchExprValueFactory,…

797d2a8

… update tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Minor fix.

4a76716

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

MaxKsyunz reviewed Nov 17, 2022

View reviewed changes

dai-chen reviewed Nov 23, 2022

View reviewed changes

Yury-Fridlyand mentioned this pull request Dec 2, 2022

Rework on OpenSearchDataType: parse, store and use mapping information #180

Merged

6 tasks

Yury-Fridlyand mentioned this pull request Feb 2, 2023

Rework on OpenSearchDataType: parse, store and use mapping information opensearch-project/sql#1314

Merged

6 tasks

acarbonetto approved these changes Mar 27, 2023

View reviewed changes

Yury-Fridlyand closed this Jun 29, 2023

Yury-Fridlyand deleted the dev-spike-use-mapping-ts-format branch June 29, 2023 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use mapping format for reading `date` values #169

[WIP] Use mapping format for reading `date` values #169

Yury-Fridlyand commented Nov 16, 2022 •

edited

Loading

MaxKsyunz Nov 17, 2022

Yury-Fridlyand Nov 17, 2022

Yury-Fridlyand commented Nov 17, 2022

dai-chen Nov 23, 2022

dai-chen Nov 23, 2022

Yury-Fridlyand Nov 25, 2022

dai-chen Nov 23, 2022 •

edited

Loading

Yury-Fridlyand Dec 2, 2022

Yury-Fridlyand commented Jun 29, 2023

	// TODO throw exception
	LogManager.getLogger(OpenSearchExprValueFactory.class).error(
	String.format("Can't recognize parsed value: %s, %s", parsed, parsed.getClass()));
	return new ExprStringValue(value.stringValue());

		private final Map<ExprType, BiFunction<Content, MappingEntry, ExprValue>> typeActionMap =
		new ImmutableMap.Builder<ExprType, BiFunction<Content, MappingEntry, ExprValue>>()

		namedAggregatorList.forEach(agg -> builder.put(agg.getName(), new MappingEntry(null, null, agg.type())));
		groupByList.forEach(group -> builder.put(group.getNameOrAlias(), new MappingEntry(null, null, group.type())));

[WIP] Use mapping format for reading date values #169

[WIP] Use mapping format for reading date values #169

Conversation

Yury-Fridlyand commented Nov 16, 2022 • edited Loading

Description

Test data

Test queries

Unsupported formats

Time interpreting issue

TBD

TODO

Links

Issues Resolved

Check List

MaxKsyunz Nov 17, 2022

Choose a reason for hiding this comment

Yury-Fridlyand Nov 17, 2022

Choose a reason for hiding this comment

Yury-Fridlyand commented Nov 17, 2022

dai-chen Nov 23, 2022

Choose a reason for hiding this comment

dai-chen Nov 23, 2022

Choose a reason for hiding this comment

Yury-Fridlyand Nov 25, 2022

Choose a reason for hiding this comment

dai-chen Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Yury-Fridlyand Dec 2, 2022

Choose a reason for hiding this comment

Yury-Fridlyand commented Jun 29, 2023

[WIP] Use mapping format for reading `date` values #169

[WIP] Use mapping format for reading `date` values #169

Yury-Fridlyand commented Nov 16, 2022 •

edited

Loading

dai-chen Nov 23, 2022 •

edited

Loading